Fix incorrect pseudocode for #[repr(C)] struct alignment #766

kinseytamsin · 2020-02-22T18:39:43Z

Alternative to #559 incorporating some suggested changes to that PR.

@RalfJung I felt like your pseudocode approached actual code too much, and sorta requires understanding what's going on with Layout in order to follow, so I just took the route of making the padding calculation into an implicit function per other suggestions.

rkanati · 2020-02-22T18:40:13Z

Neat! I'll nix mine.

kinseytamsin · 2020-02-22T18:45:26Z

@rkanati I assumed that the fact that the original PR stalled meant that you weren't likely to show up, I kinda wanted you to be able to fix yours and get the credit 😭 But I also wanted it done, so I guess I haven't really done anything wrong XD

rkanati · 2020-02-22T18:47:10Z

No problem! It's just a doc fix, after all.

RalfJung · 2020-02-23T16:52:51Z

I agree now that it's better not to use Layout, to keep this code self-contained.

However, could you please define padding_needed_for? As we have seen, it is somewhat subtle to get right.

RalfJung · 2020-02-23T16:53:23Z

src/type-layout.md


    struct[field].offset = current_offset;

    current_offset += field.size;
 }

-struct.size = current_offset + current_offset % struct.alignment;
+struct.size = current_offset + padding_needed_for(current_offset, struct.alignment);


@rkanati you didn't touch this one in your PR... do you remember why? Was that an oversight, or was the original code actually correct for some reason?

As far as I can tell, that was just an oversight.

I think so, too. It would just have been good to get explicit confirmation.

kinseytamsin · 2020-02-23T19:19:31Z

@RalfJung Yeah, I started including a definition of the function but wasn't sure whether that would be necessary. Luckily now that I know about Layout (and even which method to look at), I can just copy that code :)

kinseytamsin · 2020-02-24T00:18:20Z

Okay so, for reference, here's the code for Layout::padding_needed_for.

pub fn padding_needed_for(&self, align: usize) -> usize {
    let len = self.size();

    // Rounded up value is:
    //   len_rounded_up = (len + align - 1) & !(align - 1);
    // and then we return the padding difference: `len_rounded_up - len`.
    //
    // We use modular arithmetic throughout:
    //
    // 1. align is guaranteed to be > 0, so align - 1 is always
    //    valid.
    //
    // 2. `len + align - 1` can overflow by at most `align - 1`,
    //    so the &-mask with `!(align - 1)` will ensure that in the
    //    case of overflow, `len_rounded_up` will itself be 0.
    //    Thus the returned padding, when added to `len`, yields 0,
    //    which trivially satisfies the alignment `align`.
    //
    // (Of course, attempts to allocate blocks of memory whose
    // size and padding overflow in the above manner should cause
    // the allocator to yield an error anyway.)

    let len_rounded_up = len.wrapping_add(align).wrapping_sub(1)
        & !align.wrapping_sub(1);
    len_rounded_up.wrapping_sub(len)
}

I'm feeling like this might be a bit overkill to directly adapt into what's supposed to be pseudocode. @RalfJung do you think I should use this directly for the definition of padding_needed_for in the pseudocode or should I write a definition according to a naive definition of the algorithm (i.e., without the measures to account for extreme values/overflowing)?

I'm actually having a bit of trouble fully understanding what's going on in that code, what with all the layered modular arithmetic and bitwise operations (I'm not really fully acquainted with the identities and such to be able to easily follow what bitwise operations are doing, so after puzzling it out a bit it's turning out to be extremely non-obvious to me how to translate that into a simple presentation of the algorithm suited to pseudocode). Maybe @pnkfelix can help since they wrote that code?

RalfJung · 2020-02-24T11:57:04Z

I was actually thinking of using the implementation from the other PR:

fn align_to_padding(offset: usize, alignment: usize) -> usize { // or whatever type seems appropriate?
    let misalignment = current_offset % alignment;
    if misalignment > 0 {
        current_offset - misalignment + alignment // round up to next multiple of `alignment`
    } else {
        current_offset // already a multiple of `alignment`
    }
}

Maybe also add a note somewhere that we are ignoring overflow concerns here, and link to Layout for people that actually want to do these computations in real code.

kinseytamsin · 2020-02-24T14:08:14Z

@RalfJung Yeah, I'll do that. I'm definitely overthinking this, heh.

kinseytamsin · 2020-02-24T15:04:35Z

@RalfJung I added a function definition to the pseudocode as requested. I agree that the note directing the reader to Layout::padding_needed_for is a good idea, but should it be a note or a warning? I figured I'd ask for a second opinion before I actually add that, to be sure.

RalfJung · 2020-02-24T15:29:37Z

What do you mean, "warning"? What would it warn about?

RalfJung · 2020-02-24T15:30:52Z

Oh you mean, the fact that we ignore overflows. Yeah seems best to me to call it a warning, we don't want people to copy-paste this code.

src/type-layout.md

kinseytamsin · 2020-02-24T16:16:48Z

@RalfJung Any further concerns with this PR? 🙂 (Besides the need to squash these commits before merging.)

src/type-layout.md

RalfJung · 2020-02-24T16:32:00Z

So the code looks all right to me now, but I'd prefer to get this confirmed by some data layout guru... summoning @gnzlbg @hanna-kruppe @eddyb

kinseytamsin · 2020-02-24T17:04:18Z

@RalfJung All your review comments should be addressed now 🙂

I just edited the existing commits, since the requested changes were pretty minor.

RalfJung · 2020-02-24T17:13:21Z

Thanks, I think they are. :)

Now we are just waiting for some extra eyes on this.

hanna-kruppe · 2020-02-26T12:34:15Z

Looks fine to me too, thanks @kinseytamsin!

RalfJung

Looks like we are settled then.

Update books ## nomicon 7 commits in 3e6e1001dc6e095dbd5c88005e80969f60e384e1..71241f403091e021842ca8275740e44d0ab0ece1 2019-12-14 22:08:52 +0000 to 2020-02-17 17:21:36 +0100 - (minor) Add backticks around type names (rust-lang/nomicon#197) - Add book.toml (rust-lang/nomicon#185) - Rename `Alloc` to `AllocRef` (rust-lang/nomicon#188) - Lifetimes: explain how to fix destructor example (rust-lang/nomicon#195) - mention soundness (rust-lang/nomicon#194) - Fix example in FFI section Calling Rust from C (rust-lang/nomicon#193) - Removed repeated word (rust-lang/nomicon#191) ## reference 8 commits in 64239df..559e09c 2020-02-10 19:05:13 +0100 to 2020-03-02 01:17:14 +0100 - Syntax error fix (rust-lang/reference#769) - Fix incorrect pseudocode for #[repr(C)] struct alignment (rust-lang/reference#766) - Replace "Field-Less" with "Fieldless" (rust-lang/reference#768) - Removed repeated word (rust-lang/reference#767) - Update for change in const lint name. (rust-lang/reference#764) - semantic type -> resolved type (rust-lang/reference#761) - add behavior change of relative paths without `self` in 2018 edition (rust-lang/reference#757) - assignment operator expressions -> compound assignment expressions (rust-lang/reference#759) ## rust-by-example 3 commits in 32facd5522ddbbf37baf01e4e4b6562bc55c071a..db57f899ea2a56a544c8d280cbf033438666273d 2020-02-11 09:25:06 -0300 to 2020-02-18 17:46:46 -0300 - Minor typo fix in src/mod/visibility (rust-lang/rust-by-example#1309) - Don't suggest Into implements a reverse conversion (rust-lang/rust-by-example#1307) - Improve grammar in example of 'in let' section (rust-lang/rust-by-example#1308) ## embedded-book 2 commits in b2e1092bf67bd4d7686c4553f186edbb7f5f92db..b81ffb7a6f4c5aaed92786e770e99db116aa4ebd 2020-01-30 08:45:46 +0000 to 2020-02-27 08:06:04 +0000 - Setting output to `high` needs a `true` argument (rust-embedded/book#227) - Add licence notes to index.md (rust-embedded/book#226)

Update books ## nomicon 8 commits in 3e6e1001dc6e095dbd5c88005e80969f60e384e1..9f797e65e6bcc79419975b17aff8e21c9adc039f 2019-12-14 22:08:52 +0000 to 2020-03-04 03:45:13 +0100 - Rustup to latest rustc (rust-lang/nomicon#203) - (minor) Add backticks around type names (rust-lang/nomicon#197) - Add book.toml (rust-lang/nomicon#185) - Rename `Alloc` to `AllocRef` (rust-lang/nomicon#188) - Lifetimes: explain how to fix destructor example (rust-lang/nomicon#195) - mention soundness (rust-lang/nomicon#194) - Fix example in FFI section Calling Rust from C (rust-lang/nomicon#193) - Removed repeated word (rust-lang/nomicon#191) ## reference 8 commits in 64239df..559e09c 2020-02-10 19:05:13 +0100 to 2020-03-02 01:17:14 +0100 - Syntax error fix (rust-lang/reference#769) - Fix incorrect pseudocode for #[repr(C)] struct alignment (rust-lang/reference#766) - Replace "Field-Less" with "Fieldless" (rust-lang/reference#768) - Removed repeated word (rust-lang/reference#767) - Update for change in const lint name. (rust-lang/reference#764) - semantic type -> resolved type (rust-lang/reference#761) - add behavior change of relative paths without `self` in 2018 edition (rust-lang/reference#757) - assignment operator expressions -> compound assignment expressions (rust-lang/reference#759) ## rust-by-example 3 commits in 32facd5522ddbbf37baf01e4e4b6562bc55c071a..db57f899ea2a56a544c8d280cbf033438666273d 2020-02-11 09:25:06 -0300 to 2020-02-18 17:46:46 -0300 - Minor typo fix in src/mod/visibility (rust-lang/rust-by-example#1309) - Don't suggest Into implements a reverse conversion (rust-lang/rust-by-example#1307) - Improve grammar in example of 'in let' section (rust-lang/rust-by-example#1308) ## embedded-book 2 commits in b2e1092bf67bd4d7686c4553f186edbb7f5f92db..b81ffb7a6f4c5aaed92786e770e99db116aa4ebd 2020-01-30 08:45:46 +0000 to 2020-02-27 08:06:04 +0000 - Setting output to `high` needs a `true` argument (rust-embedded/book#227) - Add licence notes to index.md (rust-embedded/book#226)

Fix incorrect pseudocode for #[repr(C)] struct alignment

5426423

rkanati mentioned this pull request Feb 22, 2020

Pseudo-code on #[repr(C)] struct layout contains bugs #632

Closed

kinseytamsin requested a review from RalfJung February 22, 2020 18:57

RalfJung reviewed Feb 23, 2020

View reviewed changes

Explicitly describe padding algorithm in pseudocode

bc5c540

RalfJung reviewed Feb 24, 2020

View reviewed changes

src/type-layout.md Show resolved Hide resolved

RalfJung reviewed Feb 24, 2020

View reviewed changes

src/type-layout.md Outdated Show resolved Hide resolved

RalfJung reviewed Feb 24, 2020

View reviewed changes

src/type-layout.md Outdated Show resolved Hide resolved

kinseytamsin force-pushed the fix-repr-c-pseudocode branch from 99ee27b to 684d54c Compare February 24, 2020 16:41

kinseytamsin added 2 commits February 24, 2020 11:00

Add warning for #[repr(C)] struct alignment pseudocode

13e415a

Add doc comment for padding_needed_for fn in pseudocode

5d5b233

kinseytamsin force-pushed the fix-repr-c-pseudocode branch from 684d54c to 5d5b233 Compare February 24, 2020 17:02

RalfJung approved these changes Feb 26, 2020

View reviewed changes

RalfJung merged commit e96c8b9 into rust-lang:master Feb 26, 2020

ehuss mentioned this pull request Mar 2, 2020

Update books rust-lang/rust#69641

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect pseudocode for #[repr(C)] struct alignment #766

Fix incorrect pseudocode for #[repr(C)] struct alignment #766

kinseytamsin commented Feb 22, 2020

rkanati commented Feb 22, 2020

kinseytamsin commented Feb 22, 2020

rkanati commented Feb 22, 2020

RalfJung commented Feb 23, 2020

RalfJung Feb 23, 2020

kinseytamsin Feb 24, 2020

RalfJung Feb 24, 2020

kinseytamsin commented Feb 23, 2020

kinseytamsin commented Feb 24, 2020 •

edited

Loading

RalfJung commented Feb 24, 2020 •

edited

Loading

kinseytamsin commented Feb 24, 2020

kinseytamsin commented Feb 24, 2020

RalfJung commented Feb 24, 2020

RalfJung commented Feb 24, 2020

kinseytamsin commented Feb 24, 2020

RalfJung commented Feb 24, 2020

kinseytamsin commented Feb 24, 2020 •

edited

Loading

RalfJung commented Feb 24, 2020

hanna-kruppe commented Feb 26, 2020

RalfJung left a comment

Fix incorrect pseudocode for #[repr(C)] struct alignment #766

Fix incorrect pseudocode for #[repr(C)] struct alignment #766

Conversation

kinseytamsin commented Feb 22, 2020

rkanati commented Feb 22, 2020

kinseytamsin commented Feb 22, 2020

rkanati commented Feb 22, 2020

RalfJung commented Feb 23, 2020

RalfJung Feb 23, 2020

Choose a reason for hiding this comment

kinseytamsin Feb 24, 2020

Choose a reason for hiding this comment

RalfJung Feb 24, 2020

Choose a reason for hiding this comment

kinseytamsin commented Feb 23, 2020

kinseytamsin commented Feb 24, 2020 • edited Loading

RalfJung commented Feb 24, 2020 • edited Loading

kinseytamsin commented Feb 24, 2020

kinseytamsin commented Feb 24, 2020

RalfJung commented Feb 24, 2020

RalfJung commented Feb 24, 2020

kinseytamsin commented Feb 24, 2020

RalfJung commented Feb 24, 2020

kinseytamsin commented Feb 24, 2020 • edited Loading

RalfJung commented Feb 24, 2020

hanna-kruppe commented Feb 26, 2020

RalfJung left a comment

Choose a reason for hiding this comment

kinseytamsin commented Feb 24, 2020 •

edited

Loading

RalfJung commented Feb 24, 2020 •

edited

Loading

kinseytamsin commented Feb 24, 2020 •

edited

Loading